Websocket sansio implementataion #2060
Conversation
|
If you make the pipeline green, I can review it. |
|
sure |
|
@Kludex Could you please review it. all checks are passing now |
There was a problem hiding this comment.
I'm going to explain +- how is going to be my review process, so you can try to speed it up:
- I'll run the test suite, and check the coverage, and then I'll try to understand why some lines are not covered.
- I'll check the type hints, and make sure that they make sense.
- There are some tests that only work for
wsprotoor forwebsockets. I'm going to make sure that we are also running this protocol implementation where it makes sense.
I was looking at this task as well, but as gourav-kandoria already did it I can help with pointing out few semantics or if I see something else. If you don't mind 😄 |
| if self.handshake_initiated and not self.close_sent: | ||
| self.queue.put_nowait({"type": "websocket.disconnect", "code": 1006}) | ||
|
|
||
| def data_received(self, data: bytes) -> None: |
There was a problem hiding this comment.
https://websockets.readthedocs.io/en/stable/howto/sansio.html
if I'm following this right after receiving data you should call receive_eof()
There was a problem hiding this comment.
@valentin994 @Kludex we don't have to call receive_eof() after receiving data. Because, protocol ensures that (https://docs.python.org/3/library/asyncio-protocol.html#asyncio.Protocol.data_received) data_received will only be called only when the data is there. In case when eof is received from client side, the transport will close and connection_lost will be called.
|
okay sure, got it. till then I will also go through the code at my end. |
|
FYI, if you want to speed up the review process here, I need the lines on the report for the added file to be covered, or to understand why they aren't. 
I'll eventually come to this PR even if you don't do it, but it may take some time. |
| self.conn.receive_data(data) | ||
| except Exception: | ||
| self.logger.exception("Exception in ASGI server") | ||
| self.transport.close() |
There was a problem hiding this comment.
This lines(117-119) are not being covered. Because, I guess in the test suite there not any test which after connection establishment sends data which causes receive_data to throw exception such as data which don't follow websocket spec.
There was a problem hiding this comment.
We still need to test it. It's unlikely that we want Exception there as well.
As it won't be propagated as an event by the websockets sansIO protocol
This reverts commit 808f951.
As websockets sansio protocol is not propagating this event after receiving data
Kludex
force-pushed
the
websocket-sansio-implementataion
branch
from
12bb2d2
to
64d6eb7
Compare
…/gourav-kandoria/uvicorn into websocket-sansio-implementataion
|
Why? |
|
oh my bad. branch got deleted by mistake. Due to bandwidth issue, I won't be able to contribute on this. That's why closed the pr. |
|
cc @aaugustin |
This PR provides the implementation of websocket protocol using websockets sansI/O layer.
task details - #1908
Checklist for @Kludex
data_receiveshouldn't be usingException. What is the right exception there?max_sizeis being tested, and if not, test it. EDIT: I've checked it - it's not being tested.WebSocketsSansIOProtocoltest formax_size.CONTandPONGrelated code?--ws websockets-sansio.websocketslegacy version?UserWarningsaying that it's experimental, and asking users to comment in a discussion X?